Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VarDumper] Add dd() helper == dump() + exit() #26970

Merged
merged 1 commit into from Apr 19, 2018

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

By popular demand, I feel like we should reconsider our refusal for a dd() global helper.
For past references, see #26965, #26906, #13657, #17267, #19096.

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We gave some reasons to not add this function in the past ... but when you receive so many requests from the community to add it, it's time to rethink your decisions. Nicolas, thanks for reconsidering this and for adding the function.

@onEXHovia
Copy link
Contributor

I think there will be a similar number of pros and cons.
Should we add TwigFunction by analogy with dump 🤔

@pamil
Copy link
Contributor

pamil commented Apr 18, 2018

Seeing dd() anywhere in the code for the first time would cause nothing else but confusion. If one wants to save keystrokes, why not create a macro in IDE (or use a debugger)?

@linaori
Copy link
Contributor

linaori commented Apr 18, 2018

  • It doesn't add complexity
  • It's optional
  • It's faster than hooking up the debugger
  • Not everyone has experience with a debugger
  • It doesn't replace a debugger and you're not forced to use this

I see no downsides and maintenance isn't an issue 👍

@kunicmarko20
Copy link

kunicmarko20 commented Apr 18, 2018

@iltar But you have dump(), as @pamil said you can create IDE macro. If this gets accepted won't it start an avalanche of people wanting shortcuts for something?

@javiereguiluz javiereguiluz changed the title [VarDumper] Add dd() helper == dump() + die() [VarDumper] Add dd() helper == dump() + exit() Apr 18, 2018
@lsmith77 lsmith77 self-requested a review April 18, 2018 09:43
Copy link
Contributor

@lsmith77 lsmith77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the potential for harm is very small and the feature has been requested a lot.

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Pierstoval
Copy link
Contributor

Creating a macro in the IDE is part of the doc, not the framework, and not asked as much as dd(). I'm totally approving this feature as it's almost like "everyone's asking for it", where debugger usage, even though better, can be cumbersome for lots of devs.

stof
stof previously requested changes Apr 18, 2018
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SourceContextProvider must be updated to whitelist dd as well when looking for the source of the dump. Otherwise, the dump will be reported as coming from inside dd which is useless.

@linaori
Copy link
Contributor

linaori commented Apr 18, 2018

@kunicmarko20 Not everyone is always developing with an IDE, don't forget the notepad(++), Vim, atom etc developers. Also important to note down, is that the var-dumper can be used as stand-alone. Meaning if devs like the symfony dump(), they can use this package. This reaches far beyond experienced developers with debuggers, as you'll probably be reaching hobby devs, wordpress/joomla/drupal devs etc.

When you're comparing the full feature set of the var-dumper component vs other dumpers, then a dump and die functionality is quite common and desired. It takes a few lines to add the function, but adds a big feeling of completion for the developers that would otherwise have used a dd()-similar function from another framework.

I think it's quite nice to have a "full" solution coming from Symfony, making it even better 👍

@pamil
Copy link
Contributor

pamil commented Apr 18, 2018

After reading the reasoning behind introducing this function I think it's a good idea in fact and with almost no potential for harm. 👍

@deflock
Copy link

deflock commented Apr 18, 2018

The feature itself is must have, but not sure about name, confuses a bit.

@iltar Can you explain "It's optional"? What if I want dump() but not dd() in global scope?

@Meroje
Copy link

Meroje commented Apr 18, 2018

@nicolas-grekas @DojoGeekRA chrome 65 removed the need for an error code to render the preview that was introduced with 62 https://bugs.chromium.org/p/chromium/issues/detail?id=785050

@haydenk
Copy link

haydenk commented Apr 18, 2018

Why not add an optional argument to dump() to exit when it's done instead of creating a whole new function?

@lyrixx
Copy link
Member

lyrixx commented Apr 18, 2018

Why not add an optional argument to dump() to exit when it's done instead of creating a whole new function?

because dump is variadic

@haydenk
Copy link

haydenk commented Apr 18, 2018

@lyrixx Ah, good point.

@joubertredrat
Copy link

joubertredrat commented Apr 18, 2018

Oh, a enem..., erm, a friend @taylorotwell here? Hey, let's drink coffee?

About function, I think that is best to be more verbosity, how dump_die, dump_exit or dump_output as example, what you think guys?

Why not add an optional argument to dump() to exit when it's done instead of creating a whole new function?

Because dump today support multiple arguments.

@kunicmarko20
Copy link

If people think we need a shortcut then it should be small, dd is the best choice for this. If it is longer then I can write dump();die(); myself.

@Pierstoval
Copy link
Contributor

What a mess of a conversation just for a DX initiative that has been demanded for a long time, and still lots of arguments against.

I think this is part of the small changes that can really conciliate lots of dev communities and improve "classic" debug experience as well for people not using complex xdebug setups or full-stack IDEs. I mean, when one debugs using VIM, a simple dd() is really straightforward.

@akalongman
Copy link

+1 for dump_die()

@Majkl578
Copy link
Contributor

Maybe rename Symfony\Component\VarDumper\VarDumper::dump() to D::d() instead?

Copy link
Contributor

@carusogabriel carusogabriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍿

@antoniocambados
Copy link

I don't miss a dd() shortcut but I can barely think of any serious objection. The proposed name is convenient and matches other frameworks, so I'd stick with it.

@fabpot
Copy link
Member

fabpot commented Apr 19, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit a55916a into symfony:master Apr 19, 2018
fabpot added a commit that referenced this pull request Apr 19, 2018
…s-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[VarDumper] Add dd() helper == dump() + exit()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

By popular demand, I feel like we should reconsider our refusal for a `dd()` global helper.
For past references, see #26965, #26906, #13657, #17267, #19096.

Commits
-------

a55916a [VarDumper] Add dd() helper == dump() + die()
@Wirone
Copy link
Contributor

Wirone commented Apr 19, 2018

@Pierstoval

I mean, when one debugs using VIM, a simple dd() is really straightforward.

Unless they're in command mode 😂

@sstok
Copy link
Contributor

sstok commented Apr 19, 2018

Lets be clear, a function/method/class should always have a name that describes it's purpose, it helps the developers and prevents mistakes.

dd, a or Foobar are not descriptive (unless you have a Bar named foo :trollface: - insert Jonh Taffer meme here ) but it's important to understand why we use these undescriptive names, because it doesn't matter. Foobar is only used as an example for testing, dd() is something you only use during development!! It prints the result and stops execution, please don't tell me your production does this kind of logic 😐 (you properly use a proper solution for a specific problem and let the framework handle exceptions and the response handling).

Clean Code is about production code and tests, not about simple hacks to find out why something isn't working. Not everyone has the power of a debugger, sometimes the debugger is not able to reach the code (actually happened to me a few time) or doesn't show all details (or the details are to verbose), by using the VarDumper you can easily solve this problem, and having a simple method to dump and prevent further execution is perfectly fine for development and troubleshooting.

@gragio
Copy link

gragio commented Apr 19, 2018

Good job! Thanks @nicolas-grekas

@jpauli
Copy link

jpauli commented Apr 19, 2018

👍

@nicolas-grekas
Copy link
Member Author

I'm locking the thread because the issue has been resolved.
Thank you all for participating!

@symfony symfony locked as resolved and limited conversation to collaborators Apr 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet